add validation for name and description for model model group and connector resources #3805

dhrubo-os · 2025-04-29T19:29:42Z

add validation for name and description for model model group and connector resources

Description

resolved #3639

Integ test will fail as code coverages is failing for previously merged PR.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

ylwu-amzn · 2025-04-29T19:36:52Z

common/src/main/java/org/opensearch/ml/common/transport/connector/MLCreateConnectorRequest.java

+
+        if (modelName != null && !isSafeText(modelName)) {
+            exception = addValidationError(
+                "Model connector name can only contain letters, digits, spaces, underscores (_), hyphens (-), and dots (.)",


This error message is different with MLUpdateConnectorRequest.java one. Should we keep them consistent ?

"Model connector name can only contain letters, digits, spaces, underscores (_), hyphens (-), and dots (.). Max length: 1000 characters.",

Yes, in the next revision I kept it consistent.

ylwu-amzn · 2025-04-29T19:37:48Z

common/src/main/java/org/opensearch/ml/common/transport/connector/MLUpdateConnectorRequest.java

+
+            if (modelName != null && !isSafeText(modelName)) {
+                exception = addValidationError(
+                    "Model connector name can only contain letters, digits, spaces, underscores (_), hyphens (-), and dots (.). Max length: 1000 characters.",


Don't see where validate Max length: 1000 characters." , Is that built-in logic in isSafeText?

My bad, I removed that restriction from the regex. But then I forgot to remove that from the error message.

Initially I was thinking to provide 1000 characters restriction.

What do you think? Should we add such kind of restriction?

Ignore the comment, find that internal logic in isSafeText

ylwu-amzn · 2025-04-29T19:39:23Z

common/src/main/java/org/opensearch/ml/common/transport/model/MLUpdateModelRequest.java

+        String modelName = updateModelInput.getName();
+        String description = updateModelInput.getDescription();
+
+        if (modelName != null && !isSafeText(modelName)) {


Similar checking in multiple places ? Can we build some common util method ?

Yeah, I thought about that and felt too lazy to refactor 😄. But I fixed it in the second revision. Thanks for insisting on the highest standards!

rithin-pullela-aws · 2025-05-07T20:03:29Z

common/src/main/java/org/opensearch/ml/common/utils/StringUtils.java

@@ -60,6 +63,9 @@ public class StringUtils {
        + "      return input;"
        + "\n    }\n";

+    // Regex allows letters, digits, spaces, hyphens, underscores, and dots.
+    private static final String SAFE_INPUT_REGEX = "^[a-zA-Z0-9 _\\-\\.:,'()]+$";


Just curious how we determined the regex is this a standard regex generally used? I believe we want to prevent <, >, ;, ", /, \, =.
Since we are using this for description, maybe we need to allow !, @ etc?

That's a very good question. I didn't find any generic standard for the description field. So I think I'll go forward with this regex pattern: "^[\\p{L}\\p{N}\\s.,!?():@\\-_'\"]*$" that allows:

Unicode letters (\p{L})
Unicode numbers (\p{N})
Whitespace (\s)
Common punctuation (.,!?():@)
Hyphen (\-)
Underscore (_)
Single and double quotes ('")

Restricts dangerous characters like:

< and > (prevents HTML/XML tags)
/ and \ (prevents path traversal)
& (prevents HTML entities)

(prevents some SQL injection patterns)
= (prevents some query string manipulations)
; (prevents command injection)

Please let me know what do you guys think. I'm open to suggestion as well.

Sounds good, allowing unicode makes a lot of sense. I think this regex is good. The same regex will apply for name field as well right?

…nector resources Signed-off-by: Dhrubo Saha <[email protected]>

…undnancy Signed-off-by: Dhrubo Saha <[email protected]>

Signed-off-by: Dhrubo Saha <[email protected]>

dhrubo-os requested review from b4sjoo, mingshl, jngz-es, model-collapse, rbhavna, ylwu-amzn, zane-neo, Zhangxunmt, austintlee, HenryL27 and xinyual as code owners April 29, 2025 19:29

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 19:31 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 19:31 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 19:31 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 19:31 — with GitHub Actions Failure

ylwu-amzn reviewed Apr 29, 2025

View reviewed changes

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 20:45 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 20:45 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 20:45 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 20:45 — with GitHub Actions Failure

dhrubo-os force-pushed the input_validation branch from 0326043 to d8eeede Compare April 29, 2025 20:49

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 20:50 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 20:50 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 20:50 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 23:25 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 23:25 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 23:25 — with GitHub Actions Failure

dhrubo-os temporarily deployed to ml-commons-cicd-env May 7, 2025 19:15 — with GitHub Actions Inactive

rithin-pullela-aws reviewed May 7, 2025

View reviewed changes

dhrubo-os temporarily deployed to ml-commons-cicd-env May 7, 2025 20:17 — with GitHub Actions Inactive

dhrubo-os temporarily deployed to ml-commons-cicd-env May 8, 2025 17:09 — with GitHub Actions Inactive

dhrubo-os added 5 commits May 8, 2025 10:10

add validation for name and description for model model group and con…

ee513eb

…nector resources Signed-off-by: Dhrubo Saha <[email protected]>

addressing comment, added a common method in utils to remove code red…

8f389c4

…undnancy Signed-off-by: Dhrubo Saha <[email protected]>

making more flexible the regex pattern

14512bf

Signed-off-by: Dhrubo Saha <[email protected]>

removing restriction of 1000 characters

2c1546a

Signed-off-by: Dhrubo Saha <[email protected]>

addressing comments

e33b553

Signed-off-by: Dhrubo Saha <[email protected]>

dhrubo-os force-pushed the input_validation branch from ec8b889 to e33b553 Compare May 8, 2025 17:40

dhrubo-os had a problem deploying to ml-commons-cicd-env May 8, 2025 17:41 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env May 8, 2025 17:41 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env May 8, 2025 17:41 — with GitHub Actions Failure

fixing unit test messages

2d3892e

Signed-off-by: Dhrubo Saha <[email protected]>

dhrubo-os had a problem deploying to ml-commons-cicd-env May 8, 2025 18:01 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env May 8, 2025 18:01 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env May 8, 2025 18:01 — with GitHub Actions Error

fixing unit test assertion

7ccfab7

Signed-off-by: Dhrubo Saha <[email protected]>

dhrubo-os temporarily deployed to ml-commons-cicd-env May 8, 2025 18:12 — with GitHub Actions Inactive

dhrubo-os had a problem deploying to ml-commons-cicd-env May 8, 2025 19:14 — with GitHub Actions Failure

dhrubo-os temporarily deployed to ml-commons-cicd-env May 8, 2025 19:14 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add validation for name and description for model model group and connector resources #3805

add validation for name and description for model model group and connector resources #3805

dhrubo-os commented Apr 29, 2025 •

edited

Loading

ylwu-amzn Apr 29, 2025 •

edited

Loading

dhrubo-os Apr 29, 2025

ylwu-amzn Apr 29, 2025

dhrubo-os Apr 29, 2025

ylwu-amzn Apr 29, 2025

ylwu-amzn Apr 29, 2025

dhrubo-os Apr 29, 2025

rithin-pullela-aws May 7, 2025

dhrubo-os May 8, 2025

rithin-pullela-aws May 8, 2025

dhrubo-os May 8, 2025

add validation for name and description for model model group and connector resources #3805

Are you sure you want to change the base?

add validation for name and description for model model group and connector resources #3805

Conversation

dhrubo-os commented Apr 29, 2025 • edited Loading

Description

Related Issues

Check List

ylwu-amzn Apr 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhrubo-os commented Apr 29, 2025 •

edited

Loading

ylwu-amzn Apr 29, 2025 •

edited

Loading